Filter Enhancement (#933) - Filter Value Dropdown by the-dcruz · Pull Request #1061 · apache/superset

the-dcruz · 2016-09-02T19:34:42Z

Overview

This PR address the issue #933, which is to replace the filter TextField with a FreeFormSelect to make it easier for the user to know what types of values are possible for a filter.

If their desired value is not in the list, they can still type in their custom value. (Especially common for regex operation)

The feature must be enabled on the table/datasource.

Technical Changes

Added new API endpoint /filter/<datasource_type>/<datasource_id>/<column>/
Added new function values_for_column to models.DruidDatasource and models.SqlaTable (migration script included)
- values_for_column uses the same querystring as query
- Currently limited to 500 values
Added new boolean column to datasources and tables to indicate allowing filter value select
add_filter in explore.jsx will now fix the filter IDs upon render instead of by prepForm on query submit

Screenshots

How filter looks:

Option on table:

TODO for Future PR

Entire filter can be redone in React
Cache filter endpoint

@mistercrunch

the-dcruz · 2016-09-02T19:35:55Z

This PR is the same as #948

Sorry I closed that PR while I was fixing issues after rebasing and my force push didn't allow me to re-open it.

kkalyan · 2016-09-06T14:46:11Z

Great enhancement, thanks @the-dcruz!

Is there a way to select multiple options ?
Currently comma separated values works as OR. May be switch to Select2 widget ?

mistercrunch · 2016-09-11T14:47:03Z

It needs to be backward compatible with the existing multi-value filters.

the-dcruz · 2016-09-12T17:42:18Z

@kkalyan @mistercrunch
I currently have this implemented with a freeform select since it is likely that not all possible values for a given column will be displayed (limit=500 currently). My idea was to have this select act as a guide for the user to see a sample of the possible values for the column. The user can then type the values with commas as before.

Maybe a better implementation I can try would be a freeform multiselect?

the-dcruz · 2016-09-12T20:28:31Z

Updated PR to now use freeform multiselect.

ShengyaoQian · 2016-09-15T16:59:42Z

This would be a very helpful enhancement!

the-dcruz · 2016-09-15T17:56:30Z

@mistercrunch I was wondering if there was still a chance of this PR being looked into. I've periodically rebased it but will hold off until I know it will be looked at. Thanks

mistercrunch · 2016-09-21T15:51:29Z

Oh nice! I didn't see your updates since my last comment. I'm a bit overwhelmed with the amount of notifications and communication channels around me.

This looks good, I'll fetch your branch and do some testing on our staging environment today.

mistercrunch · 2016-09-21T15:54:24Z

Quick question before I test, we recently had a PR to fix a bug around filtering on strings that include commas, is that handled on your side?

mistercrunch · 2016-09-21T15:55:42Z

@vera-liu , since you're familiar with this feature and this part of the code, would you mind doing some testing/reviewing here?

the-dcruz · 2016-09-21T15:57:16Z

@mistercrunch Doesn't ring a bell. Could you point me to the PR? I'll try to get that integrated. Thanks for taking a look btw!

And no worries, I understand it can't be easy managing multiple popular open source projects 😄

vera-liu · 2016-09-21T21:00:50Z

Hi! I was trying to test this on energy_usage example database but somehow the values are not showing up for me. Am I on the right user flow?

Toggle 'Enable Filter Select' in Edit mode of energy_usage
Go to explore view and click on plus-sign of Filters

the-dcruz · 2016-09-21T21:15:55Z

@vera-liu I use the same time parameters for this query as specified for the slice. Can you show me which time params you selected?

vera-liu · 2016-09-21T21:18:43Z

These ones?

the-dcruz · 2016-09-21T21:19:25Z

Yes. Does 7 Days ago actually return any data to display in the chart?

vera-liu · 2016-09-21T21:21:36Z

Yes:

But the filter values do not show up in the dropdown:

the-dcruz · 2016-09-21T21:30:43Z

@vera-liu I see my issue. Energy usage does not have any time column.

I guess I'll ignore time in my query if there is no time column. I'll add that fix in a bit. Could you try with something time sensitive like "birth names"?

the-dcruz · 2016-09-21T22:02:54Z

@vera-liu I pushed the fix so it should work for the energy table now. I'll rebase as soon as you think things look good. Thanks!

vera-liu · 2016-09-21T23:11:01Z

It's looking good :). We used to have a bug with comma in filter values:
#1045

Solved in this PR: #1084

We should include that fix in this PR

vera-liu · 2016-09-21T23:19:51Z

caravel/models.py

+                timestamp >= text(dttm_col.dttm_sql_literal(from_dttm)),
+                timestamp <= text(dttm_col.dttm_sql_literal(to_dttm)),
+            ]
+            qry = qry.where(and_(*time_filter)) if granularity else qry


Why do we need if granularity else qry here? If this is inside if granularity: it should always be true right?

Good catch. Forgot to remove this.

the-dcruz · 2016-09-22T00:03:33Z

@vera-liu I decided that the most consistent way to integrate your fix is to wrap all the filter choices with the single quotes.

This seems reasonable to me since this is a dropdown containing column values instead of column names like many of the other dropdowns.

vera-liu · 2016-09-22T18:09:08Z

Do you mind rebasing on latest master since it has python test for filter values with commas?

the-dcruz · 2016-09-22T18:16:49Z

@vera-liu Sure. Done

the-dcruz · 2016-09-22T18:18:25Z

Looks like another migration file was introduced after rebase, causing the tests to fail. I'll update my migration file and rebase again.

the-dcruz · 2016-09-22T19:10:44Z

Tests look good after rebase @vera-liu

vera-liu · 2016-09-26T22:22:10Z

So Sorry for the delay, I think it's looking good! I'll test it against prod db and then confirm. Could you resolve the conflicts and bump the migration in the meantime? Thank you for the patience! @the-dcruz

mistercrunch · 2016-10-07T17:35:13Z

please rebase so that we can merge this!

kkalyan · 2016-11-04T15:40:02Z

@the-dcruz can't wait to see this in master, please rebase!

the-dcruz · 2016-11-08T01:42:43Z

@kkalyan Sorry about the wait! I had abandoned this PR thinking it was going to be waived off until exploreV2.

@mistercrunch I've rebased and retested this.

…taining possible column values

ascott · 2016-11-17T02:45:17Z

@vera-liu would you make sure this gets added to the filters component in V2?

vera-liu · 2016-11-18T18:39:21Z

Seems like caravel-superset renaming caused conflicts in this PR, I will try to include this feature in explore V2, but feel free to merge into V1 if you would like @the-dcruz, it's an awesome feature to have!

mistercrunch · 2017-06-12T20:46:47Z

This is done in master

Bumps [ssri](https://github.com/npm/ssri) from 6.0.1 to 6.0.2. **This update includes a security fix.** - [Release notes](https://github.com/npm/ssri/releases) - [Changelog](https://github.com/npm/ssri/blob/v6.0.2/CHANGELOG.md) - [Commits](npm/ssri@v6.0.1...v6.0.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

the-dcruz force-pushed the filter-value-selector branch from 88c37de to 18cd7c1 Compare September 12, 2016 20:25

the-dcruz force-pushed the filter-value-selector branch from 18cd7c1 to 1d6923d Compare September 12, 2016 20:40

xrmx mentioned this pull request Sep 15, 2016

Getting distinct values for filter fields #1111

Closed

vera-liu reviewed Sep 21, 2016

View reviewed changes

the-dcruz force-pushed the filter-value-selector branch from 73b1553 to b6e0b25 Compare September 21, 2016 23:55

the-dcruz force-pushed the filter-value-selector branch from 1623d7b to c872f7a Compare September 22, 2016 01:29

the-dcruz force-pushed the filter-value-selector branch 2 times, most recently from 7ded4c4 to e539882 Compare September 22, 2016 18:12

the-dcruz force-pushed the filter-value-selector branch from e539882 to 91c2280 Compare September 22, 2016 18:25

vera-liu mentioned this pull request Sep 27, 2016

Explore control panel - Chart control, TimeFilter, GroupBy, Filters #1205

Merged

the-dcruz force-pushed the filter-value-selector branch 3 times, most recently from 32cd717 to a8c3bbb Compare November 8, 2016 01:35

the-dcruz force-pushed the filter-value-selector branch from a8c3bbb to 8191486 Compare November 8, 2016 03:09

Filter Enhancement (apache#933) - Replaced text field with select con…

9f6f544

…taining possible column values

the-dcruz force-pushed the filter-value-selector branch from 8191486 to 9f6f544 Compare November 8, 2016 05:10

mistercrunch closed this Jun 12, 2017

Conversation

the-dcruz commented Sep 2, 2016

Overview

Technical Changes

Screenshots

TODO for Future PR

Uh oh!

the-dcruz commented Sep 2, 2016

Uh oh!

kkalyan commented Sep 6, 2016

Uh oh!

mistercrunch commented Sep 11, 2016

Uh oh!

the-dcruz commented Sep 12, 2016

Uh oh!

the-dcruz commented Sep 12, 2016

Uh oh!

ShengyaoQian commented Sep 15, 2016

Uh oh!

the-dcruz commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mistercrunch commented Sep 21, 2016

Uh oh!

mistercrunch commented Sep 21, 2016

Uh oh!

mistercrunch commented Sep 21, 2016

Uh oh!

the-dcruz commented Sep 21, 2016

Uh oh!

vera-liu commented Sep 21, 2016

Uh oh!

the-dcruz commented Sep 21, 2016

Uh oh!

vera-liu commented Sep 21, 2016

Uh oh!

the-dcruz commented Sep 21, 2016

Uh oh!

vera-liu commented Sep 21, 2016

Uh oh!

the-dcruz commented Sep 21, 2016

Uh oh!

the-dcruz commented Sep 21, 2016

Uh oh!

vera-liu commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vera-liu Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

the-dcruz Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

the-dcruz commented Sep 22, 2016

Uh oh!

vera-liu commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-dcruz commented Sep 22, 2016

Uh oh!

the-dcruz commented Sep 22, 2016

Uh oh!

the-dcruz commented Sep 22, 2016

Uh oh!

vera-liu commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mistercrunch commented Oct 7, 2016

Uh oh!

kkalyan commented Nov 4, 2016

Uh oh!

the-dcruz commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascott commented Nov 17, 2016

Uh oh!

vera-liu commented Nov 18, 2016

Uh oh!

mistercrunch commented Jun 12, 2017

Uh oh!

the-dcruz commented Sep 15, 2016 •

edited

Loading

vera-liu commented Sep 21, 2016 •

edited

Loading

vera-liu commented Sep 22, 2016 •

edited

Loading

vera-liu commented Sep 26, 2016 •

edited

Loading

the-dcruz commented Nov 8, 2016 •

edited

Loading